Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update gosec #2933

Merged
merged 5 commits into from
Oct 28, 2024
Merged

chore: update gosec #2933

merged 5 commits into from
Oct 28, 2024

Conversation

gartnera
Copy link
Member

@gartnera gartnera commented Sep 27, 2024

Description

All my changes were merged upstream. Continue using our fork to avoid docker hub rate limiting. We really should just use the gosec plugin on golangci-lint.

Main impact is a continued reduction in G115 false-negatives and false-positives. We will need to review the new errors before merge.

Summary by CodeRabbit

  • New Features

    • Enhanced P2P diagnostics with periodic peer discovery and communication.
    • Improved transaction receipt checking for Bitcoin transactions.
    • Updated ticker interval handling for inbound and outbound observers.
  • Bug Fixes

    • Enhanced error handling and logging across various methods for better clarity and robustness.
  • Documentation

    • Added security comments to clarify the handling of positive values and improve code readability.
  • Chores

    • Updated dependencies for security scanning tools and workflows.

@gartnera gartnera added the no-changelog Skip changelog CI check label Sep 27, 2024
Copy link
Contributor

coderabbitai bot commented Sep 27, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces several significant updates across multiple files, primarily focusing on enhancing security practices, improving error handling, and refining logging mechanisms. Key changes include updates to the gosec security scanning tool, the introduction of periodic P2P diagnostics, and enhancements to various transaction processing methods. Additionally, comments indicating security considerations have been added to multiple locations, clarifying the intent behind certain code behaviors. Overall, the changes aim to bolster the robustness and clarity of the codebase.

Changes

File Path Change Summary
.github/workflows/sast-linters.yml Updated gosec action version; refined nosec_alert job to improve detection/reporting of #nosec annotations; added PR labeling for detected #nosec usages.
cmd/zetaclientd/p2p_diagnostics.go Enhanced RunDiagnostics function with a ticker for periodic peer discovery and communication; improved error handling and logging.
e2etests/test_bitcoin_std_deposit.go Added assertions to TestBitcoinStdMemoDeposit for positive amountSatoshis and retained existing balance checks.
e2e/runner/setup_solana.go Added security comment regarding ChainID assignment.
pkg/contracts/ton/gateway.go Updated parseInbound method to handle operation codes; refined error handling in parseDeposit and parseDepositAndCall.
pkg/crypto/aes256_gcm.go Added comment addressing a false positive in DecryptAES256GCM.
pkg/memo/codec_compact.go Added security comment in packLength method regarding range checks.
pkg/ticker/ticker.go Renamed SecondsFromUint64 to DurationFromUint64Seconds for clarity; added security comment.
precompiles/logs/logs.go Reformatted AddLog function for readability; retained security comment regarding block height.
precompiles/staking/staking.go Enhanced error handling in staking methods; added security comments.
rpc/backend/node_info.go Improved error handling and comments in Syncing and SetEtherbase methods.
rpc/backend/tx_info.go Added comments indicating positive values in GetTransactionReceipt method.
rpc/namespaces/ethereum/debug/api.go Enhanced error handling and added security comments in various profiling methods.
rpc/types/utils.go Updated FormatBlock function to clarify positive value assumptions; added timestamp field.
scripts/gosec.sh Updated Docker image version for gosec tool.
zetaclient/chains/bitcoin/observer/inbound.go Improved error handling and logging in WatchInbound and ObserveInbound methods.
zetaclient/chains/evm/observer/inbound.go Updated ticker interval handling in WatchInbound and related methods.
zetaclient/chains/evm/observer/outbound.go Enhanced logging and error handling in WatchOutbound and ProcessOutboundTrackers.
zetaclient/chains/ton/observer/inbound.go Updated ticker interval handling in watchInbound method.
zetaclient/orchestrator/orchestrator.go Improved error handling and logging in ScheduleCctxSolana and related methods.
zetaclient/types/dynamic_ticker.go Added security comments to NewDynamicTicker and UpdateInterval methods.
zetaclient/zetacore/client_worker.go Added security comment in UpdateAppContextWorker method regarding interval safety.

Possibly related PRs

  • ci: Add Semgrep to CI #2912: The addition of the Semgrep workflow is related to the main PR's focus on enhancing security practices, particularly in the context of code scanning and alerting mechanisms.
  • ci: Add SARIF upload to GitHub Security Dashboard #2929: This PR enhances the CI process by uploading SARIF results to the GitHub Security Dashboard, which aligns with the main PR's emphasis on improving security alerting and reporting mechanisms.

Suggested labels

breaking:cli

Suggested reviewers

  • fbac
  • kingpinXD
  • swift1337
  • lumtis
  • brewmaster012
  • skosito

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the ci Changes to CI pipeline or github actions label Sep 27, 2024
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 9 lines in your changes missing coverage. Please review.

Project coverage is 64.47%. Comparing base (ef764ae) to head (3ec03d0).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
pkg/ticker/ticker.go 0.00% 3 Missing ⚠️
zetaclient/chains/evm/observer/inbound.go 0.00% 2 Missing ⚠️
zetaclient/chains/ton/observer/inbound.go 0.00% 2 Missing ⚠️
zetaclient/types/dynamic_ticker.go 66.66% 1 Missing ⚠️
zetaclient/zetacore/client_worker.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2933   +/-   ##
========================================
  Coverage    64.47%   64.47%           
========================================
  Files          412      412           
  Lines        28976    28985    +9     
========================================
+ Hits         18681    18687    +6     
- Misses        9512     9515    +3     
  Partials       783      783           
Files with missing lines Coverage Δ
pkg/contracts/ton/gateway.go 55.19% <100.00%> (+0.29%) ⬆️
pkg/crypto/aes256_gcm.go 76.00% <100.00%> (+0.32%) ⬆️
pkg/memo/codec_compact.go 98.21% <ø> (ø)
precompiles/logs/logs.go 77.50% <100.00%> (+0.57%) ⬆️
precompiles/staking/staking.go 44.97% <100.00%> (+0.16%) ⬆️
zetaclient/chains/bitcoin/observer/inbound.go 29.70% <ø> (ø)
zetaclient/chains/evm/observer/outbound.go 54.54% <100.00%> (+0.11%) ⬆️
zetaclient/orchestrator/orchestrator.go 23.64% <ø> (ø)
zetaclient/types/dynamic_ticker.go 60.86% <66.66%> (-1.04%) ⬇️
zetaclient/zetacore/client_worker.go 0.00% <0.00%> (ø)
... and 3 more

@gartnera gartnera marked this pull request as ready for review October 28, 2024 18:14
Copy link

!!!WARNING!!!
nosec detected in the following files: cmd/zetaclientd/p2p_diagnostics.go, e2e/e2etests/test_bitcoin_std_deposit.go, e2e/runner/setup_solana.go, pkg/contracts/ton/gateway.go, pkg/crypto/aes256_gcm.go, pkg/memo/codec_compact.go, pkg/ticker/ticker.go, precompiles/logs/logs.go, precompiles/staking/staking.go, rpc/backend/node_info.go, rpc/backend/tx_info.go, rpc/namespaces/ethereum/debug/api.go, rpc/types/utils.go, zetaclient/chains/bitcoin/observer/inbound.go, zetaclient/chains/evm/observer/outbound.go, zetaclient/orchestrator/orchestrator.go, zetaclient/types/dynamic_ticker.go, zetaclient/zetacore/client_worker.go

Be very careful about using #nosec in code. It can be a quick way to suppress security warnings and move forward with development, it should be employed with caution. Suppressing warnings with #nosec can hide potentially serious vulnerabilities. Only use #nosec when you're absolutely certain that the security issue is either a false positive or has been mitigated in another way.

Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203
Broad #nosec annotations should be avoided, as they can hide other vulnerabilities. The CI will block you from merging this PR until you remove #nosec annotations that do not target specific rules.

Pay extra attention to the way #nosec is being used in the files listed above.

@github-actions github-actions bot added the nosec label Oct 28, 2024
@gartnera gartnera enabled auto-merge October 28, 2024 18:15
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Outside diff range and nitpick comments (13)
.github/workflows/sast-linters.yml (2)

Line range hint 82-87: Consider using an array for excluded files.

The current file exclusion logic could be made more maintainable by using an array of excluded files.

-            # Skip this workflow file
-            if [ "$file" == ".github/workflows/sast-linters.yml" ] || [ "$file" == "changelog.md" ]; then
-              echo "Skipping nosec check for $file"
-              continue
-            fi
+            # Define excluded files
+            excluded_files=(".github/workflows/sast-linters.yml" "changelog.md")
+            if [[ " ${excluded_files[@]} " =~ " ${file} " ]]; then
+              echo "Skipping nosec check for excluded file: $file"
+              continue
+            fi

Line range hint 143-145: Enhance error reporting for #nosec validation.

The regex pattern and error message could be more descriptive to help developers understand and fix issues.

-          echo "$DIFF" | grep -P '#nosec(?!(\sG\d{3}))(?![^\s\t])([\s\t]*|$)' && echo "nosec without specified rule found!" && exit 1 || exit 0
+          # Detect #nosec annotations that don't specify a rule (e.g., G401)
+          if echo "$DIFF" | grep -P '#nosec(?!(\sG\d{3}))(?![^\s\t])([\s\t]*|$)'; then
+            echo "ERROR: Found #nosec annotations without specific rule numbers."
+            echo "Please specify the exact rule(s) being suppressed (e.g., #nosec G401)."
+            exit 1
+          fi
pkg/ticker/ticker.go (1)

186-186: Fix typo in function comment.

There's a typo in the comment: "secodns" should be "seconds".

-// DurationFromUint64Seconds converts uint64 of secodns to time.Duration.
+// DurationFromUint64Seconds converts uint64 of seconds to time.Duration.
zetaclient/chains/ton/observer/inbound.go (1)

Line range hint 16-18: Enhance documentation for MaxTransactionsPerTick

While the constant's purpose is clear, consider adding more detailed documentation explaining:

  • The rationale behind the 100 transaction limit
  • Performance implications
  • What happens to remaining transactions

Consider updating the comment to:

-// MaxTransactionsPerTick is the maximum number of transactions to process on a ticker
+// MaxTransactionsPerTick limits the number of transactions processed per tick to 100
+// to prevent processing delays and ensure consistent performance. Remaining transactions
+// will be processed in subsequent ticks.
pkg/contracts/ton/gateway.go (1)

127-127: Enhance documentation for security annotations

While the #nosec G115 annotations are correctly placed where type safety is guaranteed by the code structure, it would be beneficial to document the reasoning in more detail. This helps future maintainers understand why these cases are safe.

Consider expanding the comments to explain the safety guarantees:

-// #nosec G115 always in range
+// #nosec G115 -- Safe: op value is validated by the switch statement below which handles unknown cases

Also applies to: 142-142, 151-151

cmd/zetaclientd/p2p_diagnostics.go (2)

Line range hint 158-217: Enhance P2P communication robustness and maintainability.

Several improvements can be made to the peer communication implementation:

  1. Define message buffer size as a constant:
+const maxMessageSize = 1024 // Maximum size of P2P diagnostic messages
-	buf := make([]byte, 1024)
+	buf := make([]byte, maxMessageSize)
  1. Use defer for stream closure consistently:
 	stream, err := host.NewStream(context.Background(), peer.ID, protocol.ID(ProtocolID))
 	if err != nil {
 		startLogger.Error().Err(err).Msgf("fail to create stream to peer %s", peer)
 		continue
 	}
+	defer func() {
+		if err := stream.Close(); err != nil {
+			startLogger.Warn().Err(err).Msgf("fail to close stream to peer %s", peer)
+		}
+	}()
  1. Define a structured message format:
type DiagnosticMessage struct {
    Round    int    `json:"round"`
    FromID   string `json:"from_id"`
    ToID     string `json:"to_id"`
}

These changes will improve code maintainability and reduce error handling complexity.


Line range hint 218-229: Consider implementing metrics for P2P diagnostics.

The diagnostic results could be exposed as metrics to enable monitoring and alerting.

Consider adding the following metrics:

  • Peer discovery success rate
  • Ping-pong latency
  • Number of active peers
  • Message exchange success rate

This would enable better observability of the P2P network health.

precompiles/staking/staking.go (1)

Line range hint 441-447: Clean up disabled methods implementation.

The disabled methods contain unreachable code after the return statement. While this code is currently commented with //nolint:govet, it would be cleaner to either:

  1. Remove the unreachable code until the methods are re-enabled, or
  2. Comment out the return statement and keep the implementation if re-enabling is imminent.

Also, consider expanding the comment to include a brief reason for the disabling:

-// Disabled until further notice, check https://github.com/zeta-chain/node/issues/3005.
+// These staking operations are temporarily disabled due to [brief reason].
+// Track re-enabling at https://github.com/zeta-chain/node/issues/3005.

Also applies to: 476-482, 511-517

rpc/backend/node_info.go (1)

84-87: LGTM! Consider enhancing the security comment.

The security comments addressing gosec G115 are appropriate since block heights in blockchain context are indeed always positive. However, consider making the comment more descriptive for future maintainers.

Consider enhancing the comment to:

-// #nosec G115 block height always positive
+// #nosec G115 block height is guaranteed to be positive in blockchain context
rpc/types/utils.go (1)

136-153: LGTM! Consider adding documentation about value guarantees.

The added security comments correctly suppress gosec G115 warnings for values that are guaranteed to be positive in the blockchain context. However, it would be beneficial to add a brief documentation comment explaining why these values are guaranteed to be positive, making it clearer for future maintainers.

Consider adding a documentation comment at the start of the FormatBlock function:

// FormatBlock creates an ethereum block from a tendermint header and ethereum-formatted
// transactions.
// Note: The following fields are guaranteed to be positive:
// - block height: Blockchain heights start from 0 and only increment
// - size: Block size is calculated from actual data
// - gasLimit: Gas limits are configured as positive values
// - timestamp: Block timestamps are Unix timestamps (seconds since epoch)
zetaclient/chains/bitcoin/observer/inbound.go (1)

253-254: LGTM! Consider adding a clarifying comment.

The gosec suppression is justified as the block height in Bitcoin is guaranteed to be positive by the protocol design. Consider adding a brief comment explaining this invariant for future maintainers.

-	// #nosec G115 block height always positive
+	// #nosec G115 Bitcoin block height is guaranteed to be positive by protocol design
 	if !ob.IsBlockConfirmed(uint64(blockVb.Height)) {
zetaclient/chains/evm/observer/outbound.go (1)

Line range hint 1-453: Consider architectural improvements

Several architectural improvements could enhance the codebase:

  1. Move configuration values (timeouts, retry limits) to a central configuration
  2. Add comprehensive unit tests for error conditions
  3. Consider implementing circuit breakers for external calls
  4. Add metrics collection for monitoring outbound transaction processing
zetaclient/chains/evm/observer/inbound.go (1)

41-41: LGTM! Consider adding a comment about ticker duration.

The change to DurationFromUint64Seconds improves clarity. Consider adding a brief comment explaining the expected duration range for better maintainability.

+// Convert InboundTicker to duration, expected range: 1s to 1h
 interval := ticker.DurationFromUint64Seconds(ob.ChainParams().InboundTicker)
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ef764ae and 2457031.

📒 Files selected for processing (22)
  • .github/workflows/sast-linters.yml (1 hunks)
  • cmd/zetaclientd/p2p_diagnostics.go (1 hunks)
  • e2e/e2etests/test_bitcoin_std_deposit.go (1 hunks)
  • e2e/runner/setup_solana.go (1 hunks)
  • pkg/contracts/ton/gateway.go (1 hunks)
  • pkg/crypto/aes256_gcm.go (1 hunks)
  • pkg/memo/codec_compact.go (1 hunks)
  • pkg/ticker/ticker.go (1 hunks)
  • precompiles/logs/logs.go (1 hunks)
  • precompiles/staking/staking.go (1 hunks)
  • rpc/backend/node_info.go (1 hunks)
  • rpc/backend/tx_info.go (1 hunks)
  • rpc/namespaces/ethereum/debug/api.go (8 hunks)
  • rpc/types/utils.go (1 hunks)
  • scripts/gosec.sh (1 hunks)
  • zetaclient/chains/bitcoin/observer/inbound.go (1 hunks)
  • zetaclient/chains/evm/observer/inbound.go (2 hunks)
  • zetaclient/chains/evm/observer/outbound.go (1 hunks)
  • zetaclient/chains/ton/observer/inbound.go (2 hunks)
  • zetaclient/orchestrator/orchestrator.go (1 hunks)
  • zetaclient/types/dynamic_ticker.go (2 hunks)
  • zetaclient/zetacore/client_worker.go (1 hunks)
✅ Files skipped from review due to trivial changes (7)
  • e2e/runner/setup_solana.go
  • pkg/crypto/aes256_gcm.go
  • pkg/memo/codec_compact.go
  • precompiles/logs/logs.go
  • rpc/backend/tx_info.go
  • scripts/gosec.sh
  • zetaclient/zetacore/client_worker.go
🧰 Additional context used
📓 Path-based instructions (14)
cmd/zetaclientd/p2p_diagnostics.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

e2e/e2etests/test_bitcoin_std_deposit.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

pkg/contracts/ton/gateway.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

pkg/ticker/ticker.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

precompiles/staking/staking.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

rpc/backend/node_info.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

rpc/namespaces/ethereum/debug/api.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

rpc/types/utils.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/chains/bitcoin/observer/inbound.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/chains/evm/observer/inbound.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/chains/evm/observer/outbound.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/chains/ton/observer/inbound.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/orchestrator/orchestrator.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/types/dynamic_ticker.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

📓 Learnings (1)
zetaclient/chains/bitcoin/observer/inbound.go (4)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:131-132
Timestamp: 2024-10-08T15:34:47.578Z
Learning: ObserveInbound coverage will be improved in future refactor.
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:131-132
Timestamp: 2024-10-08T15:34:48.217Z
Learning: ObserveInbound coverage will be improved in future refactor.
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:131-132
Timestamp: 2024-10-08T15:34:48.217Z
Learning: ObserveInbound coverage will be improved in future refactor.
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:131-132
Timestamp: 2024-09-19T18:25:57.534Z
Learning: ObserveInbound coverage will be improved in future refactor.
🔇 Additional comments (10)
e2e/e2etests/test_bitcoin_std_deposit.go (1)

62-63: LGTM! The added assertions enhance input validation.

The positive check and gosec suppression are appropriate here. The #nosec G115 comment is justified as we've verified the amount is positive before the comparison.

.github/workflows/sast-linters.yml (1)

28-28: LGTM: Gosec version update aligns with PR objectives.

The update to v2.21.4-zeta2 maintains the use of the forked version while incorporating upstream changes, which should help reduce false positives/negatives in G115 rule as intended.

pkg/ticker/ticker.go (1)

186-189: Function rename improves clarity and maintainability.

The renaming from SecondsFromUint64 to DurationFromUint64Seconds and parameter from d to seconds enhances code readability by clearly indicating both the input unit and return type.

zetaclient/chains/ton/observer/inbound.go (2)

34-34: LGTM: Improved type safety in interval calculation

The change from SecondsFromUint64 to DurationFromUint64Seconds enhances type safety by making the duration conversion explicit.


Line range hint 115-115: Critical TODOs require attention

Two important architectural changes are pending:

  1. Compliance check implementation (TON: Add compliance checks #2916)
  2. Protocol contract v2 upgrade (Support TON with Protocol contract v2 architecture #2967)

Both changes are critical for security and protocol functionality. Consider prioritizing these tasks.

Let's verify the status of these issues:

Would you like assistance in implementing either of these features? I can help create detailed implementation plans or generate code snippets.

Also applies to: 200-200

✅ Verification successful

TODOs remain relevant - both issues are still open

Both architectural changes mentioned in the TODOs are still pending implementation:

The review comment is accurate in highlighting these critical tasks. The TODOs serve as important reminders for pending security and protocol enhancements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if these issues are still open and their current status
gh issue view 2916 --json title,state,labels
gh issue view 2967 --json title,state,labels

Length of output: 540

pkg/contracts/ton/gateway.go (1)

Line range hint 142-146: Well-structured error handling with clear context

The error handling has been implemented following best practices:

  • Specific error types for different failure cases
  • Proper error wrapping with context
  • Clear and actionable error messages
precompiles/staking/staking.go (1)

136-138: Verify the range of validator Status values.

While the comment indicates that the enum is always in range for uint8, it would be beneficial to add explicit validation to prevent potential overflow issues.

Let's verify the possible values of validator Status:

rpc/namespaces/ethereum/debug/api.go (1)

312-312: ⚠️ Potential issue

Add input validation for block numbers.

The block retrieval methods accept any uint64 block number without validation. While the conversion to int64 is safe, there should be checks for:

  1. Genesis block (number 0) which might require special handling
  2. Future blocks that haven't been created yet

Let's verify the maximum block height in the codebase:

Consider adding validation:

 func (a *API) GetHeaderRlp(number uint64) (hexutil.Bytes, error) {
+    if number == 0 {
+        return nil, errors.New("genesis block not supported")
+    }
+    latestHeight, err := a.backend.LatestBlockNumber()
+    if err != nil {
+        return nil, err
+    }
+    if number > uint64(latestHeight) {
+        return nil, fmt.Errorf("block #%d not found", number)
+    }
     // ... rest of the implementation
 }

Apply similar validation to GetBlockRlp, PrintBlock, and SeedHash methods.

Also applies to: 323-323, 334-334, 345-345

zetaclient/orchestrator/orchestrator.go (1)

621-621: Security comment update looks good.

The addition of G115 to the security comment is consistent with the PR objectives to continue using the forked gosec version. Both G115 (array length calculations) and G701 (type conversions) rules are being suppressed for this uint64 conversion, which is safe in this context as:

  1. OutboundScheduleInterval is a positive value controlled by chain parameters
  2. The conversion is used only for scheduling logic, not for array indexing or memory allocation

Let's verify the safety of OutboundScheduleInterval:

✅ Verification successful

Security comment suppression is appropriate and safe

The suppression of G115 and G701 for the uint64 conversion is justified because:

  1. The conversion is used only for scheduling interval calculation, not for array indexing or memory allocation
  2. The value is obtained from chain parameters and used in a controlled context for Solana outbound transaction scheduling
  3. The conversion appears in a well-defined scope with proper type checking (solObserver, ok := observer.(*solanaobserver.Observer))
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that OutboundScheduleInterval is always positive and within safe bounds

# Test: Search for OutboundScheduleInterval usage in chain parameters
rg -A 5 "OutboundScheduleInterval.*int"

Length of output: 2038


Script:

#!/bin/bash
# Let's verify the initialization and validation of OutboundScheduleInterval

# Check for validation rules and default values
rg -A 5 "DefaultOutboundScheduleInterval"

# Check for parameter validation
ast-grep --pattern 'func (p $_) Validate() error {
  $$$
}'

Length of output: 3016


Script:

#!/bin/bash
# Let's check the actual usage context of OutboundScheduleInterval in the orchestrator

# Check the code around the security comment
rg -B 10 -A 5 "#nosec G115 G701 positive" zetaclient/orchestrator/orchestrator.go

# Check for any validation or constraints on chain parameters
ast-grep --pattern 'func ($_ *ChainParams) Validate() error {
  $$$
}'

Length of output: 685

zetaclient/chains/evm/observer/inbound.go (1)

73-74: LGTM! Consistent duration handling.

The change maintains consistency with the earlier duration conversion and properly updates the ticker interval.

zetaclient/types/dynamic_ticker.go Show resolved Hide resolved
zetaclient/types/dynamic_ticker.go Show resolved Hide resolved
e2e/e2etests/test_bitcoin_std_deposit.go Show resolved Hide resolved
pkg/ticker/ticker.go Show resolved Hide resolved
pkg/contracts/ton/gateway.go Show resolved Hide resolved
cmd/zetaclientd/p2p_diagnostics.go Show resolved Hide resolved
rpc/namespaces/ethereum/debug/api.go Show resolved Hide resolved
zetaclient/chains/evm/observer/outbound.go Show resolved Hide resolved
scripts/gosec.sh Outdated Show resolved Hide resolved
zetaclient/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
pkg/ticker/ticker.go Outdated Show resolved Hide resolved
@gartnera gartnera added this pull request to the merge queue Oct 28, 2024
Merged via the queue into develop with commit 8776a6a Oct 28, 2024
34 of 35 checks passed
@gartnera gartnera deleted the update-gosec branch October 28, 2024 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking:cli ci Changes to CI pipeline or github actions no-changelog Skip changelog CI check nosec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants